fix: improve error messages for missing database tables#2775
fix: improve error messages for missing database tables#2775ivanauth wants to merge 7 commits intoauthzed:mainfrom
Conversation
Codecov Report❌ Patch coverage is ❌ Your project check has failed because the head coverage (73.66%) is below the target coverage (75.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #2775 +/- ##
==========================================
- Coverage 74.90% 73.66% -1.24%
==========================================
Files 484 493 +9
Lines 57938 60552 +2614
==========================================
+ Hits 43394 44597 +1203
- Misses 11519 12798 +1279
- Partials 3025 3157 +132 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
8ec386b to
126c8ff
Compare
miparnisari
left a comment
There was a problem hiding this comment.
Used Postman and tried all the APIs against CRDB and Spanner. Notes:
- we're missing this change for Spanner. You can use
docker-compose -f docker-compose.spanner.yaml up --buildto test. E.g.- call to WriteSchema I get
"unable to list caveats: spanner: code = \"NotFound\", desc = \"Table not found: caveat\", requestID = \"1.23ff9b5081a896f8.1.1.70.1\"",. - call to DeleteRelationships I get
"object definition doc not found". This line has to be updated to account for when the error description says "table"spicedb/internal/datastore/spanner/reader.go
Line 300 in f41f641
- call to WriteSchema I get
- in many cases, the errors returned to the user are prefixed with
error encoding zedtoken: unable to read unique IDwhich makes no sense to me, and probably not to the end user 😆 It comes from. i'm very tempted to remove thatspicedb/pkg/zedtoken/zedtoken.go
Lines 71 to 74 in 01d72a1
error encoding zedtokenand to clarify what the "unique ID" is - in spanner, if you didn't run migrations, calling Watch api gives a HTTP 400. But against CRDB, the watch service is completely disabled:
unknown service authzed.api.v1.WatchService, which means that you need to re-boot the server after you ran the migrations. I wonder if the server is supposed to be hable to handle migrations running at the same time ?
Anyway. Could you update
spicedb/internal/datastore/crdb/crdb.go
Line 576 in 3370198
- when you call ImportBulk, you get this
{
"code": 2,
"message": "statement description failed: ERROR: relation \"relation_tuple\" does not exist (SQLSTATE 42P01)",
"details": []
}, seems like we missed an error handling here:
| err, | ||
| codes.FailedPrecondition, | ||
| spiceerrors.ForReason( | ||
| v1.ErrorReason_ERROR_REASON_UNSPECIFIED, |
There was a problem hiding this comment.
nit: please create the right error code
internal/datastore/common/errors.go
Outdated
| // instructing the user to run migrations. | ||
| func NewSchemaNotInitializedError(underlying error) error { | ||
| return SchemaNotInitializedError{ | ||
| fmt.Errorf("datastore error: the database schema has not been initialized; please run \"spicedb datastore migrate\": %w", underlying), |
There was a problem hiding this comment.
| fmt.Errorf("datastore error: the database schema has not been initialized; please run \"spicedb datastore migrate\": %w", underlying), | |
| fmt.Errorf("%w. please run \"spicedb datastore migrate\", underlying), |
internal/datastore/crdb/caveat.go
Outdated
| if wrappedErr := pgxcommon.WrapMissingTableError(err); wrappedErr != nil { | ||
| return nil, datastore.NoRevision, wrappedErr |
There was a problem hiding this comment.
| if wrappedErr := pgxcommon.WrapMissingTableError(err); wrappedErr != nil { | |
| return nil, datastore.NoRevision, wrappedErr | |
| if pgxcommon.IsMissingTableError(err) { | |
| err = common.NewSchemaNotInitializedError(err) | |
| } |
to keep the same pattern as the lines before
| pgReadOnlyTransaction = "25006" | ||
| pgQueryCanceled = "57014" | ||
| pgInvalidArgument = "22023" | ||
| pgMissingTable = "42P01" |
There was a problem hiding this comment.
there are two constants with the same value defined elsewhere: postgresMissingTableErrorCode
please remove them in favor of this one
| cterr := tx.QueryRow(ctx, sql, args...).Scan(&newXID, &newSnapshot, ×tamp) | ||
| if cterr != nil { | ||
| if wrappedErr := common.WrapMissingTableError(cterr); wrappedErr != nil { | ||
| return newXID, newSnapshot, timestamp, wrappedErr |
There was a problem hiding this comment.
| return newXID, newSnapshot, timestamp, wrappedErr | |
| cterr = .... |
| }) | ||
| if err != nil { | ||
| if wrappedErr := pgxcommon.WrapMissingTableError(err); wrappedErr != nil { | ||
| return nil, wrappedErr |
There was a problem hiding this comment.
| return nil, wrappedErr | |
| err = wrappedErr |
otherwise you lose the errUnableToListNamespaces below
|
I also wonder if instead of littering all the datastore implementations with error handling, we could just prevent the server from coming up online when you have never ran E.g. here spicedb/internal/services/health/health.go Lines 112 to 115 in 70cd757 if I'm sure there are more changes required than this, perhaps an update to |
- Add TODO comment for future ERROR_REASON_DATASTORE_NOT_MIGRATED - Simplify error message format to: "%w. please run migrate" - Use IsMissingTableError + reassignment pattern (not early return) - Export PgMissingTable constant, remove duplicates from migration drivers - Add Spanner IsMissingTableError support in reader.go and caveat.go - Differentiate CRDB watch service error message for missing tables - Add ImportBulk error handling in bulk.go - Update tests for new error message format 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add TODO comment for future ERROR_REASON_DATASTORE_NOT_MIGRATED - Simplify error message format to: "%w. please run migrate" - Use IsMissingTableError + reassignment pattern (not early return) - Export PgMissingTable constant, remove duplicates from migration drivers - Add Spanner IsMissingTableError support in reader.go and caveat.go - Differentiate CRDB watch service error message for missing tables - Add ImportBulk error handling in bulk.go - Update tests for new error message format
ee34e7f to
7c53e96
Compare
|
I replaced scattered Before: 15 files with scattered IsMissingTableError checks (easy to miss spots) |
ab8dd24 to
33b2e94
Compare
3d37e2d to
d873dd4
Compare
131f022 to
7176e82
Compare
- Add TODO comment for future ERROR_REASON_DATASTORE_NOT_MIGRATED - Simplify error message format to: "%w. please run migrate" - Use IsMissingTableError + reassignment pattern (not early return) - Export PgMissingTable constant, remove duplicates from migration drivers - Add Spanner IsMissingTableError support in reader.go and caveat.go - Differentiate CRDB watch service error message for missing tables - Add ImportBulk error handling in bulk.go - Update tests for new error message format
a03d096 to
10f604e
Compare
- Add TODO comment for future ERROR_REASON_DATASTORE_NOT_MIGRATED - Simplify error message format to: "%w. please run migrate" - Use IsMissingTableError + reassignment pattern (not early return) - Export PgMissingTable constant, remove duplicates from migration drivers - Add Spanner IsMissingTableError support in reader.go and caveat.go - Differentiate CRDB watch service error message for missing tables - Add ImportBulk error handling in bulk.go - Update tests for new error message format
10f604e to
c3ce28a
Compare
When datastore operations fail due to missing database tables (typically because migrations haven't been run), users now see a clear error message: 'please ensure you have run spicedb datastore migrate' This improves the user experience by providing actionable guidance instead of raw database errors like 'relation X does not exist'. The improvement covers all reader operations across CRDB, PostgreSQL, and MySQL datastores including namespace, caveat, counter, and statistics queries.
- Add TODO comment for future ERROR_REASON_DATASTORE_NOT_MIGRATED - Simplify error message format to: "%w. please run migrate" - Use IsMissingTableError + reassignment pattern (not early return) - Export PgMissingTable constant, remove duplicates from migration drivers - Add Spanner IsMissingTableError support in reader.go and caveat.go - Differentiate CRDB watch service error message for missing tables - Add ImportBulk error handling in bulk.go - Update tests for new error message format
Replaces scattered IsMissingTableError checks (15 files, 157 lines) with a single gRPC readiness middleware that blocks ALL requests until the datastore is migrated. Key improvements: - Single point of control vs copy-pasted checks everywhere - Impossible to miss code paths - all gRPC requests gated automatically - Clear error message: "Please run 'spicedb datastore migrate'" - Cached checks (500ms) with singleflight to prevent thundering herd - Health probes bypass the gate for Kubernetes compatibility Net: -81 lines, better coverage, consistent UX.
Sort imports alphabetically to satisfy gci linter - logmw before readiness. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
acd0018 to
2eefa26
Compare
When users start SpiceDB without running migrations first, they get a confusing error like:
This doesn't tell them what went wrong or how to fix it.
Now the error message is clear and actionable:
This applies to all SQL datastores (postgres, mysql, crdb) and covers read operations, write operations, and statistics queries.
Closes #2749